Skip to content

Wire up vsock device to propolis-server#1075

Merged
papertigers merged 6 commits intomasterfrom
spr/papertigers/wire-up-vsock-device-to-propolis-server
Mar 19, 2026
Merged

Wire up vsock device to propolis-server#1075
papertigers merged 6 commits intomasterfrom
spr/papertigers/wire-up-vsock-device-to-propolis-server

Conversation

@papertigers
Copy link
Contributor

@papertigers papertigers commented Mar 9, 2026

This is the plumbing to allow virtio-socket devices to be usable in propolis-server.

Fixes: #1069

Created using jj-spr 0.1.0
Created using jj-spr 0.1.0
use propolis::vsock::proxy::VsockPortMapping;

// Port 8008 - VM Attestation RFD 605
const ATTESTATION_PORT: u16 = 8008;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We need to decide on a port!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We picked port 605!

@papertigers
Copy link
Contributor Author

Paired with @iximeow over a meet to discuss some of this PR.

Key takeaways:

  • provide configuration for propolis-cli PciPath
  • return MachineInitError for guest_cid by adjusting PciVirtioSocket::new to take a u64 and do validation.
  • One phd test is a known issue a phd VM failed to boot? #1035 (comment)
  • The other phd test is expected and okay as it is phd specific at the moment

@iximeow
Copy link
Member

iximeow commented Mar 9, 2026

by adjusting PciVirtioSocket::new to take a u64

the other option is to take a u32 in the API (this struct VirtioSocket) and avoid the risk of taking a cid with bogus upper bits. we'll still want to error for cid={0,1,2} but the API taking the same type as PciVirtioSock::new avoids the conversions (what really stood out to me is that guest_cid as u32 means we'd accept reserved bits in the API and build a device that uses a different truncated CID)

Created using jj-spr 0.1.0
@papertigers papertigers marked this pull request as ready for review March 16, 2026 21:46
@papertigers papertigers requested a review from iximeow March 16, 2026 21:48
pub struct InvalidGuestCid(u64);

#[derive(Debug, Copy, Clone)]
pub struct GuestCid(u64);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iximeow I ended up creating a type for this and then threading it through everywhere.

Created using jj-spr 0.1.0
@iximeow
Copy link
Member

iximeow commented Mar 16, 2026

(fwiw looks like your test failures are VMs failing to boot which should be sorted as of #1080, if you want to rebase over that)

Created using jj-spr 0.1.0
Copy link
Member

@iximeow iximeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

couple small comments, seems good overall, thanks! morally a +1 but i figure i'll let you get another commit in first 🙏

);

self.devices.insert(vsock.id.clone(), device.clone());
chipset.pci_attach(bdf, device);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i wanted to double-check my understanding of how the multi-function device bit would get set and currently it's only if there are multiple functions attached on the same bus/device (managed over in Slot::attach). that makes sense, but it also means this is a non-multi-function device from the guest's POV until we attach another function later on.

my read of the spec (and seems to agree with at least linux and illumos) is that it'd be legal to report this device as multi-function that happens to only implement a single function right now. but that it's also pretty low-risk for this device to suddenly become multi-function when there's another function here. so i think being not multi-function today while knowing it'll become multi-function in the future is ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I understand it from chat, this is okay to leave as is for merging?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yeah, whatever we do with the multifunction-ness of this device we can (should!) do as a separate change.

Created using jj-spr 0.1.0
Copy link
Member

@iximeow iximeow left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

seems good, lets ship it!

@papertigers papertigers merged commit 368a222 into master Mar 19, 2026
11 of 12 checks passed
@papertigers papertigers deleted the spr/papertigers/wire-up-vsock-device-to-propolis-server branch March 19, 2026 22:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Wire up vsock device to propolis-server

2 participants